-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More srtp factory improvements #26
base: master
Are you sure you want to change the base?
More srtp factory improvements #26
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 892/893 will probably trigger a reflection warning on Java 11+ and can be moved to the Java 8 code block.
/* Java 9+. PKCS11 configuration is set using the | ||
* configure method. | ||
* Use reflection so we can build with JDK 8. */ | ||
Method configureMethod = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not move this inside the null-check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I don't need/want to call Provider.getPrototype()
if getMethod("configure")
throws, as it will in Java 1.8.
{ | ||
logger.debug("Unable to construct PKCS11 provider: " + e.getMessage()); | ||
} | ||
finally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire finally block could just be the else-part of the provier-null check above and the inner try/catch made redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow? This is executed if getMethod("configure")
throws, as it will in Java 1.8.
This is a public method of a public class, which as I understand it won't trigger reflection warnings. (I don't see any warnings, myself, when I use Java 11 or 14.). Basically, if it works without needing to call The purpose of it is to let us use the Java 9+ API but still build with the Java 8 JDK, so it needs to be in the Java 9+ code block. |
Sorry, the inline comments are confusing. Wouldn't this work? public static synchronized Provider getProvider()
throws Exception
{
Provider provider = SunPKCS11CipherFactory.provider;
if (provider == null && useProvider)
{
try
{
Provider prototype = Security.getProvider("SunPKCS11");
if (prototype != null && !System.getProperty("java.version").startsWith("1.8"))
{
// Java 9+. PKCS11 configuration is set using the configure method.
// Use reflection so we can build with JDK 8.
Method configureMethod = Provider.class.getMethod("configure", String.class);
provider = (Provider)configureMethod.invoke(prototype, getConfiguration());
}
else
{
Class<?> clazz = Class.forName("sun.security.pkcs11.SunPKCS11");
if (Provider.class.isAssignableFrom(clazz))
{
// Java 8. PKCS11 configuration is passed as a constructor parameter.
Constructor<?> contructor = clazz.getConstructor(String.class);
provider = (Provider) contructor.newInstance(getConfiguration());
}
}
catch (Exception e)
{
logger.debug("Unable to construct PKCS11 provider: " + e.getMessage());
}
finally
{
if (provider == null)
useProvider = false;
else
SunPKCS11CipherFactory.provider = provider;
}
}
return provider;
} |
No, I'm pretty sure (This isn't the default setup, to be sure.) |
Okay, updated to include a version check. |
I hate doing version checks if there's any way the code can directly determine whether a feature is available - what's your objection to the existing logic? (I did a slight rearrangement to avoid getting the class object until it's needed.) |
The nested try's are confusing to read. Using exceptions as an expected outcome is bad code style and using finally as an else block is abuse. But that's just my opinion and I agree that version checks aren't a good thing either. The multi-release jars are supposed to solve things like this, but they have their own drawbacks.
|
b81f28c
to
4903647
Compare
Note I pulled out several parts of this PR into #28 ; what remains is less critical. |
No description provided.